From 73660740c8447dd3f8a65f8b68333027b4b6ffc9 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 18 Apr 2018 11:41:58 -0700 Subject: [PATCH] Move `Profile` back into `Unit` as a copy. The de-duplication of Units just doesn't work without it. --- .../compiler/context/compilation_files.rs | 2 +- src/cargo/core/compiler/context/mod.rs | 36 ++--- .../compiler/context/unit_dependencies.rs | 132 ++++++++++-------- src/cargo/core/compiler/custom_build.rs | 5 +- src/cargo/core/compiler/fingerprint.rs | 10 +- src/cargo/core/compiler/job_queue.rs | 12 +- src/cargo/core/compiler/mod.rs | 7 +- src/cargo/core/profiles.rs | 67 +-------- src/cargo/ops/cargo_clean.rs | 9 +- src/cargo/ops/cargo_compile.rs | 34 ++--- 10 files changed, 128 insertions(+), 186 deletions(-) diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index f8b0ca5c9..e44414cb3 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -425,7 +425,7 @@ fn compute_metadata<'a, 'cfg>( // Throw in the profile we're compiling with. This helps caching // panic=abort and panic=unwind artifacts, additionally with various // settings like debuginfo and whatnot. - cx.unit_profile(unit).hash(&mut hasher); + unit.profile.hash(&mut hasher); unit.mode.hash(&mut hasher); // Artifacts compiled for the host should have a different metadata diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 023804989..284515814 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -9,7 +9,7 @@ use jobserver::Client; use core::{Package, PackageId, PackageSet, Resolve, Target}; use core::{Dependency, Workspace}; -use core::profiles::{Profile, ProfileFor, Profiles}; +use core::profiles::{Profile, Profiles}; use ops::CompileMode; use util::{internal, profile, Cfg, CfgExpr, Config}; use util::errors::{CargoResult, CargoResultExt}; @@ -55,9 +55,9 @@ pub struct Unit<'a> { /// to be confused with *target-triple* (or *target architecture* ...), the target arch for a /// build. pub target: &'a Target, - /// This indicates the purpose of the target for profile selection. See - /// `ProfileFor` for more details. - pub profile_for: ProfileFor, + /// The profile contains information about *how* the build should be run, including debug + /// level, etc. + pub profile: Profile, /// Whether this compilation unit is for the host or target architecture. /// /// For example, when @@ -103,7 +103,6 @@ pub struct Context<'a, 'cfg: 'a> { incremental_env: Option, unit_dependencies: HashMap, Vec>>, - unit_profiles: HashMap, Profile>, files: Option>, } @@ -166,7 +165,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> { build_script_overridden: HashSet::new(), unit_dependencies: HashMap::new(), - unit_profiles: HashMap::new(), files: None, extra_compiler_args, }; @@ -328,15 +326,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> { let deps = build_unit_dependencies(units, self)?; self.unit_dependencies = deps; - self.unit_profiles = self.profiles.build_unit_profiles(units, self); - let files = CompilationFiles::new( - units, - host_layout, - target_layout, - export_dir, - self.ws, - self, - ); + let files = + CompilationFiles::new(units, host_layout, target_layout, export_dir, self.ws, self); self.files = Some(files); Ok(()) } @@ -490,11 +481,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { self.build_config.jobs } - pub fn incremental_args( - &self, - unit: &Unit, - profile_incremental: bool, - ) -> CargoResult> { + pub fn incremental_args(&self, unit: &Unit) -> CargoResult> { // There's a number of ways to configure incremental compilation right // now. In order of descending priority (first is highest priority) we // have: @@ -517,7 +504,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { // have it enabled by default while release profiles have it disabled // by default. let global_cfg = self.config.get_bool("build.incremental")?.map(|c| c.val); - let incremental = match (self.incremental_env, global_cfg, profile_incremental) { + let incremental = match (self.incremental_env, global_cfg, unit.profile.incremental) { (Some(v), _, _) => v, (None, Some(false), _) => false, (None, _, other) => other, @@ -571,13 +558,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> { Kind::Target => &self.target_info, } } - - /// Returns the profile for a given unit. - /// This should not be called until profiles are computed in - /// `prepare_units`. - pub fn unit_profile(&self, unit: &Unit<'a>) -> &Profile { - &self.unit_profiles[unit] - } } /// Acquire extra flags to pass to the compiler from various locations. diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index 7460f13ca..7f9eeabc3 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -18,7 +18,7 @@ use super::{Context, Kind, Unit}; use core::dependency::Kind as DepKind; use core::profiles::ProfileFor; -use core::Target; +use core::{Package, Target}; use ops::CompileMode; use std::collections::HashMap; use CargoResult; @@ -29,7 +29,13 @@ pub fn build_unit_dependencies<'a, 'cfg>( ) -> CargoResult, Vec>>> { let mut deps = HashMap::new(); for unit in roots.iter() { - deps_of(unit, cx, &mut deps, unit.profile_for)?; + // Dependencies of tests should not have `panic` set. + let profile_for = if unit.mode.is_any_test() { + ProfileFor::TestDependency + } else { + ProfileFor::Any + }; + deps_of(unit, cx, &mut deps, profile_for)?; } Ok(deps) @@ -108,16 +114,8 @@ fn compute_deps<'a, 'b, 'cfg>( }).filter_map(|(id, _)| match cx.get_package(id) { Ok(pkg) => pkg.targets().iter().find(|t| t.is_lib()).map(|t| { let mode = check_or_build_mode(&unit.mode, t); - Ok(( - Unit { - pkg, - target: t, - profile_for, - kind: unit.kind.for_target(t), - mode, - }, - profile_for, - )) + let unit = new_unit(cx, pkg, t, profile_for, unit.kind.for_target(t), mode); + Ok((unit, profile_for)) }), Err(e) => Some(Err(e)), }) @@ -138,7 +136,7 @@ fn compute_deps<'a, 'b, 'cfg>( if unit.target.is_lib() && unit.mode != CompileMode::Doctest { return Ok(ret); } - ret.extend(maybe_lib(unit, profile_for)); + ret.extend(maybe_lib(cx, unit, profile_for)); Ok(ret) } @@ -167,7 +165,7 @@ fn compute_deps_custom_build<'a, 'cfg>( target: not_custom_build, // The profile here isn't critical. We are just using this temp unit // for fetching dependencies that might have `links`. - profile_for: ProfileFor::Any, + profile: unit.profile, kind: unit.kind, mode: CompileMode::Build, }; @@ -180,13 +178,16 @@ fn compute_deps_custom_build<'a, 'cfg>( dep_build_script(unit) }) .chain(Some(( - Unit { - pkg: unit.pkg, - target: unit.target, - profile_for: ProfileFor::CustomBuild, - kind: Kind::Host, // build scripts always compiled for the host - mode: CompileMode::Build, - }, + new_unit( + cx, + unit.pkg, + unit.target, + ProfileFor::CustomBuild, + Kind::Host, // build scripts always compiled for the host + CompileMode::Build, + ), + // All dependencies of this unit should use profiles for custom + // builds. ProfileFor::CustomBuild, ))) .collect()) @@ -220,27 +221,25 @@ fn compute_deps_doc<'a, 'cfg>( // rustdoc only needs rmeta files for regular dependencies. // However, for plugins/proc-macros, deps should be built like normal. let mode = check_or_build_mode(&unit.mode, lib); - ret.push(( - Unit { - pkg: dep, - target: lib, - profile_for: ProfileFor::Any, - kind: unit.kind.for_target(lib), - mode, - }, + let unit = new_unit( + cx, + dep, + lib, ProfileFor::Any, - )); + unit.kind.for_target(lib), + mode, + ); + ret.push((unit, ProfileFor::Any)); if let CompileMode::Doc { deps: true } = unit.mode { - ret.push(( - Unit { - pkg: dep, - target: lib, - profile_for: ProfileFor::Any, - kind: unit.kind.for_target(lib), - mode: unit.mode, - }, + let unit = new_unit( + cx, + dep, + lib, ProfileFor::Any, - )); + unit.kind.for_target(lib), + unit.mode, + ); + ret.push((unit, ProfileFor::Any)); } } @@ -249,24 +248,20 @@ fn compute_deps_doc<'a, 'cfg>( // If we document a binary, we need the library available if unit.target.is_bin() { - ret.extend(maybe_lib(unit, ProfileFor::Any)); + ret.extend(maybe_lib(cx, unit, ProfileFor::Any)); } Ok(ret) } -fn maybe_lib<'a>(unit: &Unit<'a>, profile_for: ProfileFor) -> Option<(Unit<'a>, ProfileFor)> { +fn maybe_lib<'a>( + cx: &Context, + unit: &Unit<'a>, + profile_for: ProfileFor, +) -> Option<(Unit<'a>, ProfileFor)> { let mode = check_or_build_mode(&unit.mode, unit.target); unit.pkg.targets().iter().find(|t| t.linkable()).map(|t| { - ( - Unit { - pkg: unit.pkg, - target: t, - profile_for, - kind: unit.kind.for_target(t), - mode, - }, - profile_for, - ) + let unit = new_unit(cx, unit.pkg, t, profile_for, unit.kind.for_target(t), mode); + (unit, profile_for) }) } @@ -283,16 +278,15 @@ fn dep_build_script<'a>(unit: &Unit<'a>) -> Option<(Unit<'a>, ProfileFor)> { .iter() .find(|t| t.is_custom_build()) .map(|t| { + // The profile stored in the Unit is the profile for the thing + // the custom build script is running for. + // TODO: Fix this for different profiles that don't affect the + // build.rs environment variables. ( Unit { pkg: unit.pkg, target: t, - // The profile for *running* the build script will actually be the - // target the build script is running for (so that the environment - // variables get set correctly). This is overridden in - // `Profiles::build_unit_profiles`, so the exact value here isn't - // critical. - profile_for: ProfileFor::CustomBuild, + profile: unit.profile, kind: unit.kind, mode: CompileMode::RunCustomBuild, }, @@ -318,3 +312,27 @@ fn check_or_build_mode(mode: &CompileMode, target: &Target) -> CompileMode { _ => CompileMode::Build, } } + +fn new_unit<'a>( + cx: &Context, + pkg: &'a Package, + target: &'a Target, + profile_for: ProfileFor, + kind: Kind, + mode: CompileMode, +) -> Unit<'a> { + let profile = cx.profiles.get_profile( + &pkg.name(), + cx.ws.is_member(pkg), + profile_for, + mode, + cx.build_config.release, + ); + Unit { + pkg, + target, + profile, + kind, + mode, + } +} diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 3b00c478c..96638bf0f 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -120,7 +120,6 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes // package's library profile. let to_exec = to_exec.into_os_string(); let mut cmd = cx.compilation.host_process(to_exec, unit.pkg)?; - let profile = cx.unit_profile(unit).clone(); cmd.env("OUT_DIR", &build_output) .env("CARGO_MANIFEST_DIR", unit.pkg.root()) .env("NUM_JOBS", &cx.jobs().to_string()) @@ -131,8 +130,8 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes Kind::Target => cx.build_config.target_triple(), }, ) - .env("DEBUG", &profile.debuginfo.is_some().to_string()) - .env("OPT_LEVEL", &profile.opt_level.to_string()) + .env("DEBUG", &unit.profile.debuginfo.is_some().to_string()) + .env("OPT_LEVEL", &unit.profile.opt_level.to_string()) .env( "PROFILE", if cx.build_config.release { diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 100912ceb..e00b1324c 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -454,18 +454,10 @@ fn calculate<'a, 'cfg>( } else { cx.rustflags_args(unit)? }; - let profile_hash = { - let profile = cx.unit_profile(unit); - util::hash_u64(&( - profile, - unit.mode, - cx.incremental_args(unit, profile.incremental)?, - )) - }; let fingerprint = Arc::new(Fingerprint { rustc: util::hash_u64(&cx.build_config.rustc.verbose_version), target: util::hash_u64(&unit.target), - profile: profile_hash, + profile: util::hash_u64(&(&unit.profile, unit.mode, cx.incremental_args(unit)?)), // Note that .0 is hashed here, not .1 which is the cwd. That doesn't // actually affect the output artifact so there's no need to hash it. path: util::hash_u64(&super::path_args(cx, unit).0), diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 4fcaf4ea5..b6202a905 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -9,7 +9,7 @@ use crossbeam::{self, Scope}; use jobserver::{Acquired, HelperThread}; use core::{PackageId, Target}; -use core::profiles::ProfileFor; +use core::profiles::Profile; use ops::CompileMode; use util::{Config, DependencyQueue, Dirty, Fresh, Freshness}; use util::{internal, profile, CargoResult, CargoResultExt, ProcessBuilder}; @@ -48,7 +48,7 @@ struct PendingBuild { struct Key<'a> { pkg: &'a PackageId, target: &'a Target, - profile_for: ProfileFor, + profile: Profile, kind: Kind, mode: CompileMode, } @@ -410,7 +410,7 @@ impl<'a> Key<'a> { Key { pkg: unit.pkg.package_id(), target: unit.target, - profile_for: unit.profile_for, + profile: unit.profile, kind: unit.kind, mode: unit.mode, } @@ -420,7 +420,7 @@ impl<'a> Key<'a> { let unit = Unit { pkg: cx.get_package(self.pkg)?, target: self.target, - profile_for: self.profile_for, + profile: self.profile, kind: self.kind, mode: self.mode, }; @@ -444,8 +444,8 @@ impl<'a> fmt::Debug for Key<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, - "{} => {}/{:?} => {:?}", - self.pkg, self.target, self.mode, self.kind + "{} => {}/{} => {:?}", + self.pkg, self.target, self.profile, self.kind ) } } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 05c31e96a..f0605aa05 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -556,7 +556,7 @@ fn link_targets<'a, 'cfg>( let export_dir = cx.files().export_dir(unit); let package_id = unit.pkg.package_id().clone(); let target = unit.target.clone(); - let profile = cx.unit_profile(unit).clone(); + let profile = unit.profile; let unit_mode = unit.mode; let features = cx.resolve .features_sorted(&package_id) @@ -850,9 +850,8 @@ fn build_base_args<'a, 'cfg>( overflow_checks, rpath, ref panic, - incremental, .. - } = *cx.unit_profile(unit); + } = unit.profile; let test = unit.mode.is_any_test(); cmd.arg("--crate-name").arg(&unit.target.crate_name()); @@ -1022,7 +1021,7 @@ fn build_base_args<'a, 'cfg>( "linker=", cx.linker(unit.kind).map(|s| s.as_ref()), ); - cmd.args(&cx.incremental_args(unit, incremental)?); + cmd.args(&cx.incremental_args(unit)?); Ok(()) } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 615b0ac54..e7aa0c883 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -1,9 +1,6 @@ -use std::collections::HashMap; use std::{cmp, fmt, hash}; use ops::CompileMode; use util::toml::{StringOrBool, TomlProfile, U32OrBool}; -use core::compiler::Context; -use core::compiler::Unit; use core::interning::InternedString; /// Collection of all user profiles. @@ -51,7 +48,7 @@ impl Profiles { /// Retrieve the profile for a target. /// `is_member` is whether or not this package is a member of the /// workspace. - fn get_profile( + pub fn get_profile( &self, pkg_name: &str, is_member: bool, @@ -105,58 +102,6 @@ impl Profiles { self.dev.profile_for("", true, ProfileFor::Any) } } - - /// Build a mapping from Unit -> Profile for all the given units and all - /// of their dependencies. - pub fn build_unit_profiles<'a, 'cfg>( - &self, - units: &[Unit<'a>], - cx: &Context<'a, 'cfg>, - ) -> HashMap, Profile> { - let mut result = HashMap::new(); - for unit in units.iter() { - self.build_unit_profiles_rec(unit, None, cx, &mut result); - } - result - } - - fn build_unit_profiles_rec<'a, 'cfg>( - &self, - unit: &Unit<'a>, - parent: Option<&Unit<'a>>, - cx: &Context<'a, 'cfg>, - map: &mut HashMap, Profile>, - ) { - if !map.contains_key(unit) { - let for_unit = if unit.mode.is_run_custom_build() { - // The profile for *running* a custom build script is the - // target the script is running for. This allows - // `custom_build::build_work` to set the correct environment - // settings. - // - // In the case of `cargo clean -p`, it creates artificial - // units to compute filenames, without a dependency hierarchy, - // so we don't have a parent here. That should be OK, it - // only affects the environment variables used to *run* - // `build.rs`. - parent.unwrap_or(unit) - } else { - unit - }; - let profile = self.get_profile( - &for_unit.pkg.name(), - cx.ws.is_member(for_unit.pkg), - for_unit.profile_for, - for_unit.mode, - cx.build_config.release, - ); - map.insert(*unit, profile); - let deps = cx.dep_targets(unit); - for dep in &deps { - self.build_unit_profiles_rec(dep, Some(unit), cx, map); - } - } - } } /// An object used for handling the profile override hierarchy. @@ -176,7 +121,7 @@ struct ProfileMaker { impl ProfileMaker { fn profile_for(&self, pkg_name: &str, is_member: bool, profile_for: ProfileFor) -> Profile { - let mut profile = self.default.clone(); + let mut profile = self.default; if let Some(ref toml) = self.toml { merge_profile(&mut profile, toml); if profile_for == ProfileFor::CustomBuild { @@ -205,7 +150,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { } match toml.lto { Some(StringOrBool::Bool(b)) => profile.lto = Lto::Bool(b), - Some(StringOrBool::String(ref n)) => profile.lto = Lto::Named(n.clone()), + Some(StringOrBool::String(ref n)) => profile.lto = Lto::Named(InternedString::new(n)), None => {} } if toml.codegen_units.is_some() { @@ -236,7 +181,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { /// Profile settings used to determine which compiler flags to use for a /// target. -#[derive(Debug, Clone, Eq)] +#[derive(Debug, Clone, Copy, Eq)] pub struct Profile { pub name: &'static str, pub opt_level: InternedString, @@ -361,13 +306,13 @@ impl Profile { } /// The link-time-optimization setting. -#[derive(Clone, PartialEq, Eq, Debug, Hash)] +#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] pub enum Lto { /// False = no LTO /// True = "Fat" LTO Bool(bool), /// Named LTO settings like "thin". - Named(String), + Named(InternedString), } /// A flag used in `Unit` to indicate the purpose for the target. diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 64eed3682..d3692d5cd 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -50,10 +50,17 @@ pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> { for kind in [Kind::Host, Kind::Target].iter() { for mode in CompileMode::all_modes() { for profile_for in ProfileFor::all_values() { + let profile = profiles.get_profile( + &pkg.name(), + ws.is_member(pkg), + profile_for, + mode, + opts.release, + ); units.push(Unit { pkg, target, - profile_for, + profile, kind: *kind, mode, }); diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 9850101b1..cca95a779 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -28,7 +28,7 @@ use std::sync::Arc; use core::compiler::{BuildConfig, Compilation, Context, DefaultExecutor, Executor}; use core::compiler::{Kind, Unit}; -use core::profiles::ProfileFor; +use core::profiles::{ProfileFor, Profiles}; use core::resolver::{Method, Resolve}; use core::{Package, Source, Target}; use core::{PackageId, PackageIdSpec, TargetKind, Workspace}; @@ -383,11 +383,14 @@ pub fn compile_ws<'a>( let mut extra_compiler_args = HashMap::new(); let units = generate_targets( + ws, + profiles, &to_builds, filter, default_arch_kind, mode, &resolve_with_overrides, + release, )?; if let Some(args) = extra_args { @@ -552,30 +555,28 @@ impl CompileFilter { /// compile. Dependencies for these targets are computed later in /// `unit_dependencies`. fn generate_targets<'a>( + ws: &Workspace, + profiles: &Profiles, packages: &[&'a Package], filter: &CompileFilter, default_arch_kind: Kind, mode: CompileMode, resolve: &Resolve, + release: bool, ) -> CargoResult>> { let mut units = Vec::new(); // Helper for creating a Unit struct. let new_unit = |pkg: &'a Package, target: &'a Target, mode: CompileMode, profile_for: ProfileFor| { - let actual_profile_for = if profile_for != ProfileFor::Any { - profile_for - } else if mode.is_any_test() { - // Force dependencies of this unit to not set `panic`. - ProfileFor::TestDependency - } else { - profile_for - }; - let actual_mode = match mode { + let mode = match mode { CompileMode::Test => { if target.is_example() { // Examples are included as regular binaries to verify // that they compile. + // TODO: Broken - Dependencies of examples can be + // built with `panic`, which will cause `rustdoc` to + // fail with linking multiple binaries. CompileMode::Build } else { CompileMode::Test @@ -593,12 +594,14 @@ fn generate_targets<'a>( } else { default_arch_kind }; + let profile = + profiles.get_profile(&pkg.name(), ws.is_member(pkg), profile_for, mode, release); Unit { pkg, target, - profile_for: actual_profile_for, + profile, kind, - mode: actual_mode, + mode, } }; @@ -628,11 +631,10 @@ fn generate_targets<'a>( proposals.extend(default_units); if mode == CompileMode::Test { // Include the lib as it will be required for doctests. + // TODO: Broken - Dependencies won't have ProfileFor::TestDependency. if let Some(t) = pkg.targets().iter().find(|t| t.is_lib() && t.doctested()) { - proposals.push(( - new_unit(pkg, t, CompileMode::Build, ProfileFor::TestDependency), - false, - )); + proposals + .push((new_unit(pkg, t, CompileMode::Build, ProfileFor::Any), false)); } } } -- 2.30.2